Implement lazy directives for Ramble by linsword13 · Pull Request #1756 · Ramble-Project/ramble

linsword13 · 2026-09-01T21:41:43Z

This is inspired by Spack's lazy directive (spack/spack#51881).

Add in the dict descriptor (as a non-data descriptor) to support the lazy initiailization
Remove the convert_class_attributes since now the lazy execution allows us to perform copy on first access
Update the various directives to support the new semantics. Specifically, it now requires listing out all mutated dicts of a directive
Also unify the various language meta directives, so now they are all essentially DirectiveMeta
Update the mirror test since before it implicitly relied on the mutation happening both at the class and the instance level

gemini-code-assist

Code Review

This pull request refactors Ramble's directive system to support lazy evaluation of directives upon first access using a new DirectiveDictDescriptor descriptor. It removes the need for explicit class-to-instance attribute conversion and updates various base classes to use DirectiveMeta directly as their metaclass. Additionally, comprehensive tests are added to verify lazy evaluation, inheritance, and attribute isolation. The review feedback highlights a critical issue where instance-level modifications to these lazily evaluated directive dictionaries would be lost during cloning or copying. It is recommended to dynamically copy all evaluated directive dictionaries from self.__dict__ to the cloned or copied instances in both ApplicationBase.clone() and ObjectMixin.copy().

ramble-pr-bot · 2026-09-01T21:52:43Z

Ramble Performance Test Metrics

Results produced with commit: 3377deb

Test Name	Outcome	Duration (s)	Most Recent Run (s)	Last 5 Avg (s)
test_analyze_large_file	passed	1.0483	1.0590 (b7a264f)	1.0648
test_large_template_expansion	passed	1.1838	1.1979 (b7a264f)	1.3086
test_many_experiments	passed	27.0491	23.4773 (b7a264f)	24.0185
test_many_objects_defaults	passed	14.6088	12.3728 (b7a264f)	12.4969
test_matrix_filter_perf	passed	0.9941	1.0663 (b7a264f)	1.1014

codecov · 2026-09-01T22:09:56Z

Codecov Report

❌ Patch coverage is 97.78870% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.08%. Comparing base (6d999da) to head (e9a802e).
⚠️ Report is 3 commits behind head on develop.

Files with missing lines	Patch %	Lines
lib/ramble/ramble/language/language_base.py	94.23%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1756      +/-   ##
===========================================
+ Coverage    94.07%   94.08%   +0.01%     
===========================================
  Files          370      370              
  Lines        37399    37554     +155     
===========================================
+ Hits         35182    35333     +151     
- Misses        2217     2221       +4

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

linsword13 · 2026-09-01T23:18:02Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors Ramble's directive system to support lazy evaluation on first access, introducing the DirectiveDictDescriptor class and removing the eager conversion of class attributes to instance attributes across all base classes. A comprehensive test suite has been added to verify lazy evaluation, inheritance, and isolation. The review feedback highlights critical inheritance issues where using getattr on classes or objects can mistakenly retrieve parent class attributes (such as evaluated directive dictionaries or preferred versions) instead of checking the subclass's own __dict__ directly, which would bypass subclass directive execution or raise incorrect errors.

linsword13 · 2026-09-01T23:50:15Z

/gemini review

linsword13 · 2026-09-03T18:37:15Z

/gemini review

rfbgo

Two random questions:

The bot reports a slow down from this. Is it it true? If so I think there are some places we might be able to improve perf slightly (eg optimized copies)
I asked gemini to review this PR wrt "leaking" (it says it will leak). I kind of expect this is something you were already careful about, but I wanted to double check we believe this is indeed safe?

linsword13 · 2026-09-14T16:31:01Z

Two random questions:

The bot reports a slow down from this. Is it it true? If so I think there are some places we might be able to improve perf slightly (eg optimized copies)

I asked gemini to review this PR wrt "leaking" (it says it will leak). I kind of expect this is something you were already careful about, but I wanted to double check we believe this is indeed safe?

Great questions:

For 1. it does seem to cause a slowdown on some perf tests. One reason is that previously the directive initialization cost are part of the startup, which wasn't counted by the benchmarking. And now with the lazy init, these init times are now part of the operations that are being benchmarked. So this is more or less an accounting thing. Other factors include the more works that are needed during copy (as we can no longer rely on the class-level attributes.) I should do a more thorough analysis though to see if any improvements are possible, and at least have a better breakdown of the timing.

For 2. I did try to look for leaking, but I will do another pass to see where I missed.

codecov · 2026-09-14T23:32:23Z

Codecov Report

❌ Patch coverage is 96.24853% with 32 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
lib/ramble/ramble/language/language_base.py	92.46%	19 Missing ⚠️
...amble/ramble/test/language/test_lazy_directives.py	98.37%	7 Missing ⚠️
...os/builtin/base_classes/object-mixin/base_class.py	81.25%	6 Missing ⚠️

📢 Thoughts on this report? Let us know!

Convert eager directive execution into on-demand lazy evaluation via non-data descriptors, eliminating upfront convert_class_attributes deepcopies and isolating instance mutations. Signed-off-by: Lin Guo <linsword13@gmail.com>

linsword13 · 2026-09-18T17:57:21Z

Two random questions:

The bot reports a slow down from this. Is it it true? If so I think there are some places we might be able to improve perf slightly (eg optimized copies)

I asked gemini to review this PR wrt "leaking" (it says it will leak). I kind of expect this is something you were already careful about, but I wanted to double check we believe this is indeed safe?

Great questions:

For 1. it does seem to cause a slowdown on some perf tests. One reason is that previously the directive initialization cost are part of the startup, which wasn't counted by the benchmarking. And now with the lazy init, these init times are now part of the operations that are being benchmarked. So this is more or less an accounting thing. Other factors include the more works that are needed during copy (as we can no longer rely on the class-level attributes.) I should do a more thorough analysis though to see if any improvements are possible, and at least have a better breakdown of the timing.

For 2. I did try to look for leaking, but I will do another pass to see where I missed.

The perf has been improved. The leaking should hopefully be addressed as well.

rfbgo · 2026-09-18T18:04:51Z

The perf has been improved. The leaking should hopefully be addressed as well.

Awesome, thank you for being willing to take those in. I know those were none trivial changes but I think the code is stronger now

gemini-code-assist Bot reviewed Sep 1, 2026

View reviewed changes

Comment thread var/ramble/repos/builtin/base_classes/application-base/base_class.py Outdated

Comment thread var/ramble/repos/builtin/base_classes/object-mixin/base_class.py

gemini-code-assist Bot reviewed Sep 1, 2026

View reviewed changes

Comment thread lib/ramble/ramble/language/language_base.py Outdated

Comment thread lib/ramble/ramble/language/language_base.py Outdated

Comment thread lib/ramble/ramble/language/shared_language.py Outdated

linsword13 force-pushed the lazy-directives branch from 1fadb66 to 87a882c Compare September 1, 2026 23:49

linsword13 force-pushed the lazy-directives branch 2 times, most recently from 7efbad1 to 2b398d1 Compare September 3, 2026 18:29

linsword13 marked this pull request as ready for review September 4, 2026 00:35

douglasjacobsen requested a review from rfbgo September 11, 2026 14:08

rfbgo reviewed Sep 14, 2026

View reviewed changes

Comment thread lib/ramble/ramble/language/language_base.py Outdated

Comment thread lib/ramble/ramble/language/language_base.py

Comment thread var/ramble/repos/builtin/base_classes/object-mixin/base_class.py

Comment thread lib/ramble/ramble/language/language_base.py

linsword13 force-pushed the lazy-directives branch 2 times, most recently from 73a3649 to 85adeac Compare September 14, 2026 23:11

linsword13 marked this pull request as draft September 14, 2026 23:41

linsword13 force-pushed the lazy-directives branch 3 times, most recently from 812fa9d to bec5989 Compare September 16, 2026 21:38

Implement lazy directives for Ramble

3377deb

Convert eager directive execution into on-demand lazy evaluation via non-data descriptors, eliminating upfront convert_class_attributes deepcopies and isolating instance mutations. Signed-off-by: Lin Guo <linsword13@gmail.com>

linsword13 force-pushed the lazy-directives branch from bec5989 to 3377deb Compare September 16, 2026 22:57

linsword13 marked this pull request as ready for review September 18, 2026 17:57

Conversation

linsword13 commented Sep 1, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

ramble-pr-bot Bot commented Sep 1, 2026 • edited by ramble-project-pr-bot Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ramble Performance Test Metrics

Uh oh!

codecov Bot commented Sep 1, 2026

Codecov Report

Uh oh!

linsword13 commented Sep 1, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

linsword13 commented Sep 1, 2026

Uh oh!

linsword13 commented Sep 3, 2026

Uh oh!

rfbgo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

linsword13 commented Sep 14, 2026

Uh oh!

codecov Bot commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

linsword13 commented Sep 18, 2026

Uh oh!

rfbgo commented Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ramble-pr-bot Bot commented Sep 1, 2026 •

edited by ramble-project-pr-bot Bot

Loading

codecov Bot commented Sep 14, 2026 •

edited

Loading